-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ensure lifecycle tasks wait for messages to be pushed #2603
base: development/8.6
Are you sure you want to change the base?
Conversation
Hello francoisferrand,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
389da95
to
ca50af2
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
... and 1 file with indirect coverage changes
@@ Coverage Diff @@
## development/8.6 #2603 +/- ##
===================================================
+ Coverage 55.30% 55.34% +0.04%
===================================================
Files 198 198
Lines 12915 12925 +10
===================================================
+ Hits 7142 7153 +11
+ Misses 5763 5762 -1
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more. |
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command:
Alternatively, the |
return resolve(); | ||
}))); | ||
|
||
return Promise.allSettled(promises).then(() => done(), done); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this code, in case of error in _compareRulesToList
, we are ignoring the returned error, as we would have it in the "then" with the rejected
status. This looks like a behavior change we don't want? We ignore errors for _sendBucketEntry
but for _compareRulesToList
, we actually call the reject
function...
Promise.allSettled
will return an array with the status for each promise, including the ones rejected:
Promise.allSettled([
Promise.resolve(33),
new Promise((resolve) => setTimeout(() => resolve(66), 0)),
99,
Promise.reject(new Error("an error")),
]).then((values) => console.log(values), err => console.log('catch', err));
[
{
"status": "fulfilled",
"value": 33
},
{
"status": "fulfilled",
"value": 66
},
{
"status": "fulfilled",
"value": 99
},
{
"status": "rejected",
"reason": {}
}
]
(and no "catch" log).
And if we are aligned, it also means this is something we do not test, or returning the error in the callback has no impact...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right (and I will update the code), but actually _compareRulesToList
is not upposed to fail either : it calls _retryEntry
, which ignores the errors. Errors on individual objects are retried a bit ; but eventually we just skip the object (or batch), relying on the periodic scan to eventually process these obejcts.
Issue: BB-641